Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [Registry] Better search #21

Merged
merged 39 commits into from
Apr 11, 2024
Merged

feat: [Registry] Better search #21

merged 39 commits into from
Apr 11, 2024

Conversation

mohandast52
Copy link
Collaborator

@mohandast52 mohandast52 commented Apr 3, 2024

Proposed changes

Mocked data for agents list

mock
  • Network change
network-change.mp4
  • Service example
service-search.mov

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

@mohandast52 mohandast52 self-assigned this Apr 3, 2024
Copy link

vercel bot commented Apr 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
autonolas-registry ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 10, 2024 4:31pm
tokenomics ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 10, 2024 4:31pm

apps/autonolas-registry/common-util/hooks/useSubgraph.jsx Outdated Show resolved Hide resolved
apps/autonolas-registry/common-util/hooks/useSubgraph.jsx Outdated Show resolved Hide resolved
apps/autonolas-registry/components/ListAgents/index.jsx Outdated Show resolved Hide resolved
apps/autonolas-registry/components/ListAgents/index.jsx Outdated Show resolved Hide resolved
apps/autonolas-registry/common-util/hooks/useSubgraph.jsx Outdated Show resolved Hide resolved
apps/autonolas-registry/components/ListServices/index.jsx Outdated Show resolved Hide resolved
@@ -17,128 +19,176 @@ const { Title } = Typography;

export const getTableColumns = (
type,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the logic here and in fetchDataSource where we need to check type is a bit complicated - it might be hard to find an error in the code or reconsider some of the columns or data structure when all three pages' configs are so connected. do you think it's possible and makes sense to split it all into three configs for columns and dataSource?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes complete sense. I would like to refactor this in upcoming PRs to avoid too much change here 🙏

import {
Row, Col, Button, Typography,
} from 'antd';
import { Row, Col, Button, Typography } from 'antd';
import { get } from 'lodash';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import { get } from 'lodash';
import get from 'lodash/get';

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, sure. Does it add any improvements?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, there's a small difference
image

using this plugin: https://marketplace.visualstudio.com/items?itemName=ambar.bundle-size

not 100% sure if it's accurate post-build, but if it is then we should consider reducing where we can

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh, nice!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a component with a single export, please move to components folder and name Details.jsx

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently used by agents, components, and services, so I've kept it in the common-utils. Any reason to move it to components folder instead?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, so...

the components folder is for reusable components i.e. this Details component, used by multiple others

the pages folder should be reserved for specific pages that pull in reusable components to create the expected page

the common-utils or utils folders should be for helper functions, i.e. string parsers, converters, object manipulation .etc, perhaps constants, though I tend to put it outside of utils folder to seperate concerns

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your point, but my approach differs slightly:

  • page simply imports the core components. I believe if we start adding the core components here, it will become too large to maintain, especially since they tend to grow in size.
  • components is for core components such as Agents, Services, etc.
  • common-util is intended for common components that can be used by other core components. For example components that can be used in Agents, Services, etc
  • utils is reserved for constants, but we could potentially move it to common-util.

I think eventually, the truly reusable components will be moved to feature libraries in NX, such as the login component, utility functions, etc.

apps/autonolas-registry/common-util/Details/index.jsx Outdated Show resolved Hide resolved
apps/autonolas-registry/common-util/hooks/useHelpers.jsx Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove all the awaits from these functions if the response isn't processed
we can't batch them with Promise.all or Multicall otherwise

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused here. Could you provide an example?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error on my behalf, ran some tests, functions as expected, no need to remove them.
in fact, return await is now more performant?
https://eslint.org/docs/latest/rules/no-return-await

@mohandast52 mohandast52 changed the title (WIP) feat: [Registry] Better search feat: [Registry] Better search Apr 10, 2024
@mohandast52 mohandast52 merged commit 1c5c7e1 into main Apr 11, 2024
7 checks passed
@mohandast52 mohandast52 deleted the mohan/registry-search branch April 11, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants